test(mop-up): close 5 audit findings (0.8.6)#37
Merged
Conversation
Final mop-up of the 6 audit findings the author can act on without spec-level redesign. Test-only changes: sync Bulkhead Hypothesis suite, sync on_error BaseException test, three Nit fixes. One finding from the chunk-3 hand-review excluded as INVALID (audit looked in wrong file). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 Nit fixes first (cheap), 2 Low test additions (sync Bulkhead Hypothesis suite, sync on_error BaseException tests), release notes + PR. Test-only, no production changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`test_no_slot_leak_after_drain` asserted `bulkhead._sem._value == max_concurrent` after a gather of n_requests. That assertion read a CPython implementation detail of asyncio.Semaphore — non-portable across interpreters and brittle against any future asyncio refactor. Replace with a behavioral check: after gather completes, submit max_concurrent fresh acquires under a tight acquire_timeout (0.05s). If any slot leaked, the post-drain gather would block past the timeout and BulkheadFullError would surface as a deterministic failure. The `_acquire_timeout` override is still a private-attribute touch (per-instance test-local config, not a CPython internal), so the `# noqa: SLF001` annotation stays. Closes audit Nit finding (test_bulkhead_props.py:113) from planning/audit/2026-06-07-deep-audit.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ted total `assert len(budget._deposits) > 0` would have passed even with the internal lock removed and the deque corrupted, as long as ONE survivor remained. The audit's recommended tighten is to assert the exact expected total — computable from the test constants. After the 0.8.3 deposit-hoist, deposits count requests (not attempts), so `(N_SYNC_THREADS * N_OPS_PER_THREAD) + N_ASYNC_TASKS` = (4*50)+20 = 220 is the exact expected. Budget TTL is 60.0s so no purge fires within the sub-second test runtime. If the actual count diverges, the test now surfaces it. Closes audit Nit finding (test_threading_with_shared_budget.py:77) from planning/audit/2026-06-07-deep-audit.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… test `test_async_client_accepts_explicit_decoder_without_pydantic` covered the AsyncClient escape hatch (explicit `decoder=` bypasses the pydantic fail-fast). The sync Client has the same `_default_pydantic_decoder()` gate at the same point in `__init__`, but no test asserted the parallel behavior. Add `test_sync_client_accepts_explicit_decoder_without_pydantic` as the sync mirror; hoist `_FakeDecoder` to module top to keep the two tests DRY. Closes audit Nit finding (test_optional_extras_pydantic_missing.py:41) from planning/audit/2026-06-07-deep-audit.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds tests/test_bulkhead_sync_props.py mirroring the async file's three property tests, using threading.Thread + ThreadPoolExecutor + a lock-guarded shared counter instead of asyncio.gather: 1. in_flight_never_exceeds_max_concurrent (under any thread interleaving) 2. fail_fast_rejects_when_at_capacity (acquire_timeout=0 raises BulkheadFullError) 3. no_slot_leak_after_drain (behavioral check, no _value peek) Strategies match the async file: max_concurrent ∈ [1, 8], n_requests ∈ [1, 32], delay ∈ [0.001, 0.005] s. Async file is unchanged; the new file establishes sync/async parity for the resilience property-test surface. Closes audit Low finding (test_bulkhead_props.py:1, sync gap) from planning/audit/2026-06-07-deep-audit.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
test_middleware.py::test_on_error_lets_cancelled_propagate pinned the async invariant: on_error catches Exception, NOT BaseException, so asyncio.CancelledError propagates through compose_async unchanged. The sync `on_error` decorator has the same `except Exception` clause but no test asserted the parallel KeyboardInterrupt / SystemExit invariant. Add two sync mirror tests so a future regression that widens the catch to BaseException surfaces immediately. Closes audit Low finding (test_middleware.py:162 sync parity gap) from planning/audit/2026-06-07-deep-audit.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five test-only fixes close the last actionable audit findings: behavioral drain check, exact threading-budget deposit total, sync Client decoder- escape peer, sync Bulkhead Hypothesis suite, sync on_error BaseException propagation tests. No production code change, no behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lly wired `assert client is not None` was tautological — AsyncClient() either raises or returns an instance; it cannot return None. The implicit assertion was "construction did not raise ImportError," already covered by the absence of pytest.raises. Replace with `assert client._decoder is fake` — proves construction succeeded AND the explicit decoder bypassed the default-pydantic path. Applied to both async and sync tests for symmetry. The original async form pre-dated the audit; the sync mirror copied it verbatim in this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the last 5 actionable audit findings — all in the test suite. See
planning/specs/2026-06-08-test-mop-up-design.mdfor the design andplanning/releases/0.8.6.mdfor the user-facing release notes.What changes
test_no_slot_leak_after_drainis behavioral (no internals peek).test_threading_with_shared_budgetasserts the exact deposit count.test_optional_extras_pydantic_missinghas a syncClientpeer.tests/test_bulkhead_sync_props.py(Hypothesis suite for sync Bulkhead).on_errorBaseException propagation tests.Audit findings closed
test_bulkhead_props.py:113test_threading_with_shared_budget.py:77test_optional_extras_pydantic_missing.py:41test_bulkhead_sync_props.pytest_middleware_sync.pytestsOne chunk-3 finding (
test_client_methods.py has no construction tests) was excluded as INVALID — the async construction and lifecycle tests live in dedicated files, not the per-method file.Test plan
just lint-cigreen after each commit.git diff main..HEAD -- src/).Release
Tag
0.8.6from the merge SHA after this PR lands.🤖 Generated with Claude Code